Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite a few manual index loops with while-let #73910

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jul 1, 2020

There were a few instances of this pattern:

while index < vec.len() {
    let item = &vec[index];
    // ...
}

These can be indexed at once:

while let Some(item) = vec.get(index) {
    // ...
}

Particularly in ObligationForest::process_obligations, this mitigates
a codegen regression found with LLVM 11 (#73526).

There were a few instances of this pattern:

```rust
while index < vec.len() {
    let item = &vec[index];
    // ...
}
```

These can be indexed at once:

```rust
while let Some(item) = vec.get(index) {
    // ...
}
```

Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jul 1, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jul 1, 2020

⌛ Trying commit 47425e4 with merge a152931374175aa20d366c38bebdf75643c46443...

@leonardo-m
Copy link

I generally like to change the code this way, it's shorter, cleaner, and it removes a potential panic from the binary. But I've seen that performing this change inside some inner loops worsens the code performance :-/

@bors
Copy link
Contributor

bors commented Jul 1, 2020

💥 Test timed out

@cuviper
Copy link
Member Author

cuviper commented Jul 1, 2020

@bors try

@bors
Copy link
Contributor

bors commented Jul 1, 2020

⌛ Trying commit 47425e4 with merge c08b67a4d251676f43286dd6186841233746e0d2...

@cuviper
Copy link
Member Author

cuviper commented Jul 1, 2020

@leonardo-m I get that it can be fickle, but in this case it specifically made performance better, at least on LLVM 11. AFAICS it's neutral to LLVM 10, but we'll see what perf says.

@Mark-Simulacrum
Copy link
Member

Might be good to try and file an LLVM bug regardless - seems like knowing that the index is in bounds would probably be fairly common.

@bors
Copy link
Contributor

bors commented Jul 1, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: c08b67a4d251676f43286dd6186841233746e0d2 (c08b67a4d251676f43286dd6186841233746e0d2)

@rust-timer
Copy link
Collaborator

Queued c08b67a4d251676f43286dd6186841233746e0d2 with parent d462551, future comparison URL.

@cuviper
Copy link
Member Author

cuviper commented Jul 1, 2020

AFAICS it's neutral to LLVM 10

Actually, it might be an improvement. The difference I saw in #73526 (comment) was some extra setup computing the bounds, but there was still a branch calling panic_bounds_check for both LLVM 10 and 11. With this PR, that check is totally gone.

Might be good to try and file an LLVM bug regardless - seems like knowing that the index is in bounds would probably be fairly common.

OK, if I can distill that code to a reasonably small chunk of LLVM IR as a reproducer, then I'll file it.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c08b67a4d251676f43286dd6186841233746e0d2): comparison url.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 1, 2020

perf looks clean or like a slight (up to 0.5% improvement), and since it's an improvement in the code, too, I see no reason not to do this.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 1, 2020

📌 Commit 47425e4 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
Rewrite a few manual index loops with while-let

There were a few instances of this pattern:

```rust
while index < vec.len() {
    let item = &vec[index];
    // ...
}
```

These can be indexed at once:

```rust
while let Some(item) = vec.get(index) {
    // ...
}
```

Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
Rewrite a few manual index loops with while-let

There were a few instances of this pattern:

```rust
while index < vec.len() {
    let item = &vec[index];
    // ...
}
```

These can be indexed at once:

```rust
while let Some(item) = vec.get(index) {
    // ...
}
```

Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
Rewrite a few manual index loops with while-let

There were a few instances of this pattern:

```rust
while index < vec.len() {
    let item = &vec[index];
    // ...
}
```

These can be indexed at once:

```rust
while let Some(item) = vec.get(index) {
    // ...
}
```

Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2020
…arth

Rollup of 16 pull requests

Successful merges:

 - rust-lang#72569 (Remove legacy InnoSetup GUI installer)
 - rust-lang#73306 (Don't implement Fn* traits for #[target_feature] functions)
 - rust-lang#73345 (expand: Stop using nonterminals for passing tokens to attribute and derive macros)
 - rust-lang#73449 (Provide more information on duplicate lang item error.)
 - rust-lang#73569 (Handle `macro_rules!` tokens consistently across crates)
 - rust-lang#73803 (Recover extra trailing angle brackets in struct definition)
 - rust-lang#73839 (Split and expand nonstandard-style lints unicode unit test.)
 - rust-lang#73841 (Remove defunct `-Z print-region-graph`)
 - rust-lang#73848 (Fix markdown rendering in librustc_lexer docs)
 - rust-lang#73865 (Fix Zulip topic format)
 - rust-lang#73892 (Clean up E0712 explanation)
 - rust-lang#73898 (remove duplicate test for rust-lang#61935)
 - rust-lang#73906 (Add missing backtick in `ty_error_with_message`)
 - rust-lang#73909 (`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs)
 - rust-lang#73910 (Rewrite a few manual index loops with while-let)
 - rust-lang#73929 (Fix comment typo)

Failed merges:

r? @ghost
@bors bors merged commit 8fcb015 into rust-lang:master Jul 2, 2020
@cuviper
Copy link
Member Author

cuviper commented Jul 2, 2020

Might be good to try and file an LLVM bug regardless - seems like knowing that the index is in bounds would probably be fairly common.

OK, if I can distill that code to a reasonably small chunk of LLVM IR as a reproducer, then I'll file it.

It seems highly dependent on ThinLTO, which is going to make a reproducer harder. With codegen-units=1, this bounds check is totally optimized away in both LLVM 10 and 11, even before this PR. I'll poke a little more at it, but I'm not sure how to present such issues to LLVM.

@cuviper cuviper deleted the while-indexing branch August 9, 2020 23:36
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants